Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DOC] extend documentation for add_channels #13051

Merged
merged 9 commits into from
Jan 14, 2025
Merged

[DOC] extend documentation for add_channels #13051

merged 9 commits into from
Jan 14, 2025

Conversation

skjerns
Copy link
Contributor

@skjerns skjerns commented Jan 8, 2025

Fixes #11106.

What does this implement/fix?

I stumpled upon the documentation of add_channels and upon googleing saw that I was not the only one being confused on how to use the function.

Therefore, some additions that will make it easier to understand.

TODO: Some of the links are not referenced correctly, I don't know how to reference the auto examples. Can someone fix this? :)

@skjerns skjerns mentioned this pull request Jan 8, 2025
Closed
@skjerns
Copy link
Contributor Author

skjerns commented Jan 8, 2025

Sorry, don't understand the CI errors, can someone clarify?

@larsoner
Copy link
Member

larsoner commented Jan 8, 2025

A bunch of failures should be fixed by #13049

The style one is weird as it is >= 3.10 on main already. Maybe merging main into your branch once #13049 is in will fix things

@hoechenberger
Copy link
Member

hoechenberger commented Jan 13, 2025 •
edited
Loading

It's difficult for me to review because the diff extends over dozens of files. I think it's because you opened the PR for merging into the maint/1.9 branch instead of main and merged changes from main into your branch

@skjerns skjerns changed the base branch from maint/1.9 to main January 13, 2025 11:20
@skjerns
Copy link
Contributor Author

skjerns commented Jan 13, 2025

It's difficult for me to review because the diff extends over dozens of files. I think it's because you opened the PR for merging into the maint/1.9 branch instead of main and merged changes from main into your branch

yep, indeed, that seemed to have happened. I'll changed to merge into main?

@hoechenberger
Copy link
Member

It's difficult for me to review because the diff extends over dozens of files. I think it's because you opened the PR for merging into the maint/1.9 branch instead of main and merged changes from main into your branch

yep, indeed, that seemed to have happened. I'll changed to merge into main?

That would be great! I'm not sure if that works with an existing PR, though, or if you'd have to close this one and open a ne one

cbrnr
cbrnr reviewed Jan 13, 2025
View reviewed changes
mne/channels/channels.py Outdated Show resolved Hide resolved
cbrnr
cbrnr reviewed Jan 13, 2025
View reviewed changes
mne/channels/channels.py Outdated Show resolved Hide resolved
cbrnr
cbrnr reviewed Jan 13, 2025
View reviewed changes
.circleci/config.yml Outdated Show resolved Hide resolved
cbrnr
cbrnr reviewed Jan 13, 2025
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
@skjerns
Copy link
Contributor Author

skjerns commented Jan 13, 2025

I've reverted the changes, not sure how they ended up in the commit, probably because of merging 1.9 into it dev beofre.

It was a bit messy though, let me know if you want me to open a new PR to have a cleaner commit history.

@cbrnr
Copy link
Contributor

cbrnr commented Jan 13, 2025 •
edited
Loading

I've reverted the changes, not sure how they ended up in the commit, probably because of merging 1.9 into it dev beofre.

Yep!

It was a bit messy though, let me know if you want me to open a new PR to have a cleaner commit history.

No, looking good right now! We squash and merge anyway.

hoechenberger
hoechenberger reviewed Jan 13, 2025
View reviewed changes
Copy link
Member

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great, just a minor suggestion

@cbrnr
Copy link
Contributor

cbrnr commented Jan 13, 2025

Do you want to add a changelog entry? If you think this is not necessary let me know and I'll add the corresponding label here so that the test passes.

@skjerns @hoechenberger
channels.py aktualisieren
4832d6a 
Co-authored-by: Richard Höchenberger <[email protected]>
@skjerns
Copy link
Contributor Author

skjerns commented Jan 13, 2025

Do you want to add a changelog entry? If you think this is not necessary let me know and I'll add the corresponding label here so that the test passes.

Not necessary!

@drammock
Copy link
Member 

mne/channels/channels.py:697:89: E501 Line too long (97 > 88)

/home/circleci/project/mne/epochs.py:docstring of mne.channels.channels.UpdateChannelsMixin.add_channels:46: WARNING: py:class reference target not found: mne.Raw [ref.class]

(should be mne.io.Raw not mne.Raw)

sappelhoff
sappelhoff approved these changes Jan 14, 2025
View reviewed changes
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, this clarifies how to use the function by a lot!

@hoechenberger hoechenberger enabled auto-merge (squash) January 14, 2025 10:36
@hoechenberger hoechenberger merged commit 5fec4e0 into mne-tools:main Jan 14, 2025
30 checks passed
@cbrnr
Copy link
Contributor

cbrnr commented Jan 14, 2025

Thanks @skjerns!

larsoner added a commit that referenced this pull request Jan 14, 2025
@larsoner
Merge remote-tracking branch 'upstream/main' into pre-commit-ci-updat…
7881c4c 
…e-config

* upstream/main:
  [DOC] extend documentation for add_channels (#13051)
  Add `combine_tfr` to API (#13054)
  Add `combine_spectrum()` function and allow `grand_average()` to support `Spectrum` data (#13058)
  BUG: Fix bug with helium anon (#13056)
larsoner added a commit to emma-bailey/mne-python that referenced this pull request Jan 14, 2025
@larsoner
Merge remote-tracking branch 'upstream/main' into ebm
72facf0 
* upstream/main:
  [DOC] extend documentation for add_channels (mne-tools#13051)
  Add `combine_tfr` to API (mne-tools#13054)
  Add `combine_spectrum()` function and allow `grand_average()` to support `Spectrum` data (mne-tools#13058)
  BUG: Fix bug with helium anon (mne-tools#13056)
  [ENH] Add option to store and return TFR taper weights (mne-tools#12910)
qian-chu pushed a commit to qian-chu/mne-python that referenced this pull request Jan 20, 2025
@skjerns @qian-chu
[DOC] extend documentation for add_channels (mne-tools#13051)
56a1e42
larsoner added a commit to larsoner/mne-python that referenced this pull request Jan 24, 2025
@larsoner
Merge remote-tracking branch 'upstream/main' into cola5
46c193d 
* upstream/main: (57 commits)
  Allow lasso selection sensors in a plot_evoked_topo (mne-tools#12071)
  MAINT: Fix doc build (mne-tools#13076)
  BUG: Improve sklearn compliance (mne-tools#13065)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13073)
  MAINT: Add Numba to 3.13 test (mne-tools#13075)
  Bump autofix-ci/action from ff86a557419858bb967097bfc916833f5647fa8c to 551dded8c6cc8a1054039c8bc0b8b48c51dfc6ef in the actions group (mne-tools#13071)
  [BUG] Correct annotation onset for exportation to EDF and EEGLAB (mne-tools#12656)
  New feature for removing heart artifacts from EEG or ESG data using a Principal Component Analysis - Optimal Basis Sets (PCA-OBS) algorithm (mne-tools#13037)
  [BUG] Fix taper weighting in computation of TFR multitaper power (mne-tools#13067)
  [FIX] Reading an EDF with preload=False and mixed frequency (mne-tools#13069)
  Fix evoked topomap colorbars, closes mne-tools#13050 (mne-tools#13063)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13060)
  BUG: Fix bug with interval calculation (mne-tools#13062)
  [DOC] extend documentation for add_channels (mne-tools#13051)
  Add `combine_tfr` to API (mne-tools#13054)
  Add `combine_spectrum()` function and allow `grand_average()` to support `Spectrum` data (mne-tools#13058)
  BUG: Fix bug with helium anon (mne-tools#13056)
  [ENH] Add option to store and return TFR taper weights (mne-tools#12910)
  BUG: viz plot window's 'title' argument showed no effect. (mne-tools#12828)
  MAINT: Ensure limited set of tests are skipped (mne-tools#13053)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbrnr cbrnr cbrnr left review comments

@wmvanvliet wmvanvliet wmvanvliet approved these changes

@sappelhoff sappelhoff sappelhoff approved these changes

@hoechenberger hoechenberger hoechenberger approved these changes

@agramfort agramfort Awaiting requested review from agramfort agramfort is a code owner

@mscheltienne mscheltienne Awaiting requested review from mscheltienne mscheltienne is a code owner

@dengemann dengemann Awaiting requested review from dengemann dengemann is a code owner

@jasmainak jasmainak Awaiting requested review from jasmainak jasmainak is a code owner

@larsoner larsoner Awaiting requested review from larsoner

@drammock drammock Awaiting requested review from drammock

@adam2392 adam2392 Awaiting requested review from adam2392

@alexrockhill alexrockhill Awaiting requested review from alexrockhill

Assignees
No one assigned
Labels
no-changelog-entry-needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add_channels() documentation is difficult to understand
7 participants
@skjerns @larsoner @hoechenberger @cbrnr @drammock @wmvanvliet @sappelhoff